Skip to content

Add configurable Python interpreter discovery command - #4534

Closed
ting-hong-shieh wants to merge 2 commits into
facebook:mainfrom
ting-hong-shieh:fix/1662-interpreter-discovery-command
Closed

Add configurable Python interpreter discovery command#4534
ting-hong-shieh wants to merge 2 commits into
facebook:mainfrom
ting-hong-shieh:fix/1662-interpreter-discovery-command

Conversation

@ting-hong-shieh

Copy link
Copy Markdown
Contributor

Summary

  • add python-interpreter-find-cmd as a configuration-only interpreter source
  • execute the configured program directly from the config directory
  • require successful UTF-8 output containing exactly one non-empty path
  • resolve relative output paths from the config directory
  • validate mutual exclusion with other interpreter-selection options and document explicit shell usage

Root cause

Pyrefly only supported fixed interpreter paths, known environment types, and executable-name lookup. Environment managers that require a command to discover the active interpreter could not participate without first modifying the shell environment that launched Pyrefly.

The option is an argv array and does not invoke a shell implicitly. Workflows requiring shell features can opt in explicitly with sh -c, cmd /C, or PowerShell.

User impact

Projects using tools such as Poetry, direnv, or Nix can provide a reproducible interpreter-discovery command in project configuration.

Testing

  • cargo test interpreter (config, command execution, failure handling, and LSP interpreter tests)
  • formatter and Clippy through test.py

Fixes #1662

@meta-codesync

meta-codesync Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

This pull request has been imported. If you are a Meta employee, you can view this in D115850800. (Because this pull request was imported automatically, there will not be any future comments.)

@ting-hong-shieh
ting-hong-shieh marked this pull request as ready for review August 13, 2026 08:40
@github-actions
github-actions Bot requested a review from grievejia August 13, 2026 09:30

@grievejia grievejia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! I found several issues that we need to fix before landing. See inline comments. Feel free to ask if any part is unclear, or if you have other thoughts on them!

pub(crate) fallback_python_interpreter_name: Option<ConfigOrigin<String>>,

/// Command whose stdout is the path to the Python interpreter.
pub(crate) python_interpreter_find_cmd: Option<Vec<String>>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An empty array is not a valid command, but the current type accepts it and configuration parsing currently treats it as valid -- error only surfaces when Pyrefly tries to run the command.

Please reject an empty array when the configuration is read, and store a value that always contains a program plus optional arguments. This lets the execution code assume that a program is present.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. InterpreterDiscoveryCommand is a newtype with #[serde(try_from = "Vec<String>")], so an empty array is rejected when the configuration is read. The execution path now takes &[String] and can index [0] for the program.

&self,
working_directory: Option<&Path>,
) -> anyhow::Result<ConfigOrigin<PathBuf>> {
let command_parts = self

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The caller reaches this code only after it confirms that the command is present. Please pass the command slice into this method instead of reading the optional field again. This makes the required input clear and avoid the need to re-check if the optional is none -- an error case that cannot occur.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. The signature is now find_interpreter_from_command(command_parts: &[String], working_directory: Option<&Path>) -> anyhow::Result<PathBuf>. The impossible None case is gone.

{
interpreter = working_directory.join(interpreter);
}
Ok(ConfigOrigin::auto(interpreter))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The discovery command runs before find_interpreter has selected the highest-priority source. This method then marks the result as Auto. Later, the configured-source branch accepts only ConfigFile values. If a configuration contains both this command and conda-environment, Pyrefly runs the command, skips its result, and selects Conda. The validation reports a warning but does not stop this execution.

This is bad because a configured external program can run even when Pyrefly will not use its output. It also makes the real priority differ from the documented priority.

Please make this method accept the command slice and return only a PathBuf. Then call it in find_interpreter at the point where the configured command has won, before the configured Conda branch:

if let Some(command) = self.python_interpreter_find_cmd.as_deref() {
    let interpreter =
        Self::find_interpreter_from_command(command, path)?;
    return Ok(ConfigOrigin::auto(interpreter));
}

At that point, the early return has already applied the command's priority. The resolved path can remain Auto, so Pyrefly does not serialize it as an explicit python-interpreter-path.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, as you sketched. The call moved into find_interpreter with an early return, placed after the ConfigFile interpreter-path branch and before the ConfigFile Conda branch, so the command runs only once it has won. A configuration with both this command and conda-environment no longer executes the program and then discards its output. The result stays ConfigOrigin::auto.

));
};

let mut command = Command::new(program);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

current_dir sets the working directory for the child process, but it does not give a relative program path a stable base on every platform. Rust documents this case as platform-specific and unstable. For example, ["./tools/find-python"] can resolve from the configuration directory on one platform and from the directory that started Pyrefly on another.

This can make the same project configuration fail on another platform or run a different file. It also conflicts with the documentation, which says that the command runs from the configuration directory.

The existing which dependency provides which_in, which resolves the executable before the process starts. working_directory already contains the configuration directory because the caller passes self.source.root(). One possible implementation is:

let program = match working_directory {
    Some(root) => which_in(program, std::env::var_os("PATH"), root),
    None => which(program),
}
.with_context(|| "Could not resolve the interpreter discovery command")?;

let mut command = Command::new(program);

which_in keeps an absolute path, resolves a relative path that contains a separator from root, and searches PATH for a bare name. Please add a Windows test that covers this case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Resolution now happens before the process starts:

let program = match working_directory {
    Some(root) => which_in(program, std::env::var_os("PATH"), root),
    None => which(program),
}

current_dir is still set, since the command's own working directory is documented behaviour. I added the Windows test as test_interpreter_find_command_resolves_relative_program, but it is #[cfg(windows)] and I only have Linux here, so that one is unverified on my side.

Comment thread crates/pyrefly_config/src/config.rs Outdated
// file or CLI flag). If not, we auto-discover a `typings/` directory below.
let site_package_path_set = self.python_environment.site_package_path.is_some();

if self.interpreters.python_interpreter_find_cmd.is_some()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation says that all five interpreter-selection options are mutually exclusive, but the checks are split across three places. Some pairs are missed. For example, skip-interpreter-query = true together with python-interpreter-path = "./python" produces no warning, and the skip option silently wins.

This makes the documented rule unreliable. It also means that each new option needs more pair-specific conditions, such as the new python_interpreter_find_cmd.is_none() exception later in this method.

Please replace the separate checks with one check near the start of configure. Collect the names of all explicit selections, then report one warning when more than one is present:

let mut selections = Vec::new();

if matches!(
    self.interpreters.python_interpreter_path.as_ref(),
    Some(ConfigOrigin::CommandLine(_) | ConfigOrigin::ConfigFile(_))
) {
    selections.push("python-interpreter-path");
}
if self.interpreters.python_interpreter_find_cmd.is_some() {
    selections.push("python-interpreter-find-cmd");
}
if self.interpreters.fallback_python_interpreter_name.is_some() {
    selections.push("fallback-python-interpreter-name");
}
if self.interpreters.conda_environment.is_some() {
    selections.push("conda-environment");
}
if self.interpreters.skip_interpreter_query {
    selections.push("skip-interpreter-query");
}

if selections.len() > 1 {
    configure_errors.push(anyhow::anyhow!(
        "Only one interpreter selection option can be set, but found: {}.",
        selections.join(", "),
    ));
}

After this change, remove the path-versus-fallback check inside the skip_interpreter_query branch and the path-versus-Conda check near the end of configure. Extend the test with the missing skip-interpreter-query plus path case and with the command combined with each of the other four options.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. There is now a single interpreter_selections list near the start of configure that collects every explicit selection and emits one error when more than one is set. The path-versus-fallback check inside the skip_interpreter_query branch and the path-versus-Conda check near the end are both removed.

For the test I looped over all five options and asserted on every pair rather than enumerating them, which covers the skip-interpreter-query plus path case you named. That subsumes test_python_interpreter_conda_environment, so I dropped it instead of keeping it alongside — say the word if you would rather it stayed.

Setting this explicitly, especially when not using a venv, will make it difficult for your configuration
to be reused between different systems and platforms.

### `python-interpreter-find-cmd`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new public configuration option, but it is absent from the JSON schema. Editors therefore cannot complete the option or validate its value in pyrefly.toml and pyproject.toml. The root schema permits unknown properties, so the current schema test does not report the omission.

Please update these three files:

  1. Add this property beside the other interpreter options in schemas/pyrefly.json:
"python-interpreter-find-cmd": {
  "description": "A program and its arguments that print the path of the Python interpreter to query.",
  "type": "array",
  "items": {
    "type": "string"
  },
  "minItems": 1
}
  1. Add this value to schemas/test-pyrefly.toml:
python-interpreter-find-cmd = ["poetry", "env", "info", "-e"]
  1. Add the same value under [tool.pyrefly] in schemas/test-pyproject.toml.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, all three files: the property in schemas/pyrefly.json with minItems: 1, and the ["poetry", "env", "info", "-e"] value in schemas/test-pyrefly.toml and under [tool.pyrefly] in schemas/test-pyproject.toml. schemas/validate_schemas.py passes 56 tests.


#[cfg(unix)]
#[test]
fn test_find_interpreter_from_command() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test prints a constant relative path. If command.current_dir(working_directory) is removed, the command still prints the same text, and the later path-joining code still produces the expected value. The test therefore does not protect the documented rule that the command itself runs in the configuration directory.

Please keep this test as the check for relative output paths, and rename it to describe that purpose. Add a separate cross-platform test in which the command reports its real working directory:

#[cfg(any(unix, windows))]
#[test]
fn test_interpreter_find_command_uses_working_directory() {
    let tempdir = tempdir().unwrap();

    #[cfg(unix)]
    let command = ["sh", "-c", "pwd"];
    #[cfg(windows)]
    let command = ["cmd", "/C", "cd"];

    let interpreters = Interpreters {
        python_interpreter_find_cmd: Some(
            command.into_iter().map(str::to_owned).collect(),
        ),
        ..Default::default()
    };

    let interpreter = interpreters
        .find_interpreter(Some(tempdir.path()))
        .unwrap();

    assert_eq!(
        interpreter.as_path().canonicalize().unwrap(),
        tempdir.path().canonicalize().unwrap(),
    );
}

This test fails if the child working directory is no longer set. It also covers absolute command output and the Windows behavior described in the documentation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. The old test is renamed test_interpreter_find_command_resolves_relative_output and keeps its original purpose. The new test_interpreter_find_command_uses_working_directory runs pwd / cd and compares the canonicalized output against the temp directory, so it fails if current_dir is dropped.

skip_interpreter_query: true,
..
} => write!(f, "<interpreter query skipped>"),
Self {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pyrefly dump-config formats this value after interpreter discovery. With python-interpreter-find-cmd = ["poetry", "env", "info", "-e"], a successful discovery currently prints only Using interpreter: /resolved/python. This new branch does not run after success because the resolved path is then set. If the intent is to show where the path came from, please handle the state where both the command and the resolved path are set. For example: Using interpreter: interpreter at path /resolved/python (from command poetry env info -e). This would match the existing output for the fallback command. If the source is not meant to be shown, this new branch is not needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Showing the source was the intent, so I handled the both-set state rather than removing the branch. dump-config now prints interpreter at path /resolved/python (from command \poetry env info -e`)`, matching the shape of the existing fallback-command output.

Comment thread website/docs/configuration.mdx Outdated
[`python-interpreter-find-cmd`](#python-interpreter-find-cmd),
[`fallback-python-interpreter-name`](#fallback-python-interpreter-name), or
[`conda-environment`](#conda-environment) if either are set in a config file.
Both cannot be set in a config at the same time.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list now contains more than two options, so "Both" is no longer correct. Please use "Only one of these options can be set in a configuration."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — now "Only one of these options can be set in a configuration."

Allow environment-manager workflows to return an interpreter path without requiring Pyrefly to know each manager or shell environment.
Validate discovery commands when configuration is read and run them only after their interpreter source wins selection. Resolve programs relative to the config root, centralize option conflicts, and cover schema and cross-platform behavior.
@ting-hong-shieh
ting-hong-shieh force-pushed the fix/1662-interpreter-discovery-command branch from 7a80fda to c8edcbc Compare August 19, 2026 17:53
@github-actions github-actions Bot added size/xl and removed size/xl labels Aug 19, 2026
@ting-hong-shieh

Copy link
Copy Markdown
Contributor Author

@grievejia all nine comments are addressed, with a reply on each thread. The branch is also rebased onto main.

Three things the rebase decided that were not part of your review, so worth a look:

  • The call is now find_interpreter(project_root.as_deref()) rather than self.source.root(). configure_at on main derives project_root as source.root_from_file().or(project_root) and ConfigSource::root() no longer exists, so this both compiles and extends the behaviour to synthetic configurations.
  • test_python_interpreter_conda_environment is dropped, because the generalized mutual-exclusion test covers that pair.
  • In configuration.mdx, item 4 takes main's rewritten pyvenv.cfg description; only the item 3 wording is mine.

Validation at c8edcbc8, on Linux:

  • cargo test --workspace — exit 0, 8735 passed across 29 targets, 0 failed.
  • cargo clippy --workspace --all-targets — exit 0. Two warnings remain, at crates/pyrefly_config/src/error_kind.rs:627 and pyrefly/benches/micro.rs:272; neither file is touched by this branch.
  • cargo fmt --all --check — clean.
  • schemas/validate_schemas.py — 56 tests, OK.

One gap: test_interpreter_find_command_resolves_relative_program, the Windows case from the which_in thread, is #[cfg(windows)] and did not run here. It needs CI or a Windows reviewer to confirm.

Disclosure, per the AI Usage section of CONTRIBUTING.md: the rebase, the conflict resolutions, and this comment and the nine thread replies were produced by an AI agent (Claude Code) working in my checkout. I reviewed them before posting.

@stroxler stroxler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review automatically exported from Phabricator review in Meta.

@meta-codesync meta-codesync Bot closed this in 7da6e3f Aug 20, 2026
@meta-codesync meta-codesync Bot added the Merged label Aug 20, 2026
@meta-codesync

meta-codesync Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

@grievejia merged this pull request in 7da6e3f.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Add ability to set command to discover the interpreter

3 participants